Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025

User description

Benchmark PR elastic#137712

Type: Corrupted (contains bugs)

Original PR Title: Add User Profile Size Limit Enforced During Profile Updates
Original PR Description: Currently, there are no limits on the size of a user profile. Profiles store username, initials, avatars, etc.

Authorized Kibana observability clients can store an unlimited amount of data in user profile via update-profile.

This change puts a limit on profile size to avoid heap memory pressure and OOM crashes.

A new configuration setting, xpack.security.profile.max_size, was introduced with a default value of 10 MB to remain safely above the 1 MB request limit size enforced by Kibana.

Limit enforcement is implemented with a profile document read before the update, to provide a full view of the profile footprint. This approach is intended to be lightweight. Still, a document read is now incurred for every update request.
Original PR URL: elastic#137712


PR Type

Enhancement, Tests


Description

  • Add configurable user profile size limit to prevent heap exhaustion

    • New setting xpack.security.profile.max_size with 10 MB default
    • Validates combined profile size (labels + application data) on updates
  • Implement profile size validation during update operations

    • Reads profile document before update to calculate total footprint
    • Throws ElasticsearchStatusException when size exceeds limit
  • Add comprehensive unit and integration tests for quota enforcement

    • Tests profile update behavior at and beyond size limits
    • Tests helper methods for serialization and map operations

Diagram Walkthrough

flowchart LR
  A["Profile Update Request"] --> B["Read Current Profile"]
  B --> C["Calculate Combined Size"]
  C --> D{"Size < Limit?"}
  D -->|Yes| E["Apply Update"]
  D -->|No| F["Reject with Error"]
  E --> G["Return Success"]
  F --> H["Return BAD_REQUEST"]
Loading

File Walkthrough

Relevant files
Enhancement
ProfileService.java
Implement profile size limit validation logic                       

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/profile/ProfileService.java

  • Add MAX_SIZE_SETTING configuration for profile size limit (10 MB
    default)
  • Store maxProfileSize as instance variable initialized from settings
  • Implement validateProfileSize() static method to check profile
    footprint
  • Add helper methods: combineMaps(), mapFromBytesReference(),
    serializationSize()
  • Modify updateProfileData() to validate size after update via document
    read
+65/-1   
Configuration changes
Security.java
Register profile size limit setting                                           

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

  • Register ProfileService.MAX_SIZE_SETTING in security plugin settings
    list
+1/-0     
Tests
ProfileIntegTests.java
Add integration test for quota enforcement                             

x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/profile/ProfileIntegTests.java

  • Add testUpdateProfileDataHitStorageQuota() integration test
  • Configure test node with 1 KB profile size limit
  • Test profile update behavior when quota is exceeded
  • Verify ElasticsearchException is thrown on overflow
+33/-0   
ProfileServiceTests.java
Add unit tests for size validation helpers                             

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/profile/ProfileServiceTests.java

  • Add testSerializationSize() to verify size calculation
  • Add testMapFromBytesReference() for bytes conversion
  • Add testCombineMaps() for map merging logic
  • Add testValidateProfileSize() for validation behavior
  • Add helper method newBytesReference() for test utilities
+54/-0   
Documentation
137712.yaml
Add changelog entry for profile size limit                             

docs/changelog/137712.yaml

+5/-0     

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Incorrect quota enforcement

Description: The size limit check uses 'if (actualSize.compareTo(limit) >= 0)' which rejects updates
when the profile size equals the configured limit, contradicting the intent of a maximum
allowed size and enabling a trivial denial-of-service by preventing profiles from ever
reaching exactly the quota; the comparison should allow equality (use '>' instead of
'>='), or the setting/validation message should clearly state "must be strictly less
than".
ProfileService.java [270-286]

Referred Code
if (doc == null) {
    return;
}
Map<String, Object> labels = combineMaps(doc.doc.labels(), request.getLabels());
Map<String, Object> data = combineMaps(mapFromBytesReference(doc.doc.applicationData()), request.getData());
ByteSizeValue actualSize = ByteSizeValue.ofBytes(serializationSize(labels) + serializationSize(data));
if (actualSize.compareTo(limit) >= 0) {
    throw new ElasticsearchStatusException(
        Strings.format(
            "cannot update profile [%s] because the combined profile size of [%s] exceeds the maximum of [%s]",
            request.getUid(),
            actualSize,
            limit
        ),
        RestStatus.BAD_REQUEST
    );
}
TOCTOU quota bypass

Description: The update is written to the index before validating the new total profile size, briefly
allowing oversized profiles to be stored, which can be exploited by issuing concurrent
updates to exceed the configured limit and potentially cause memory/heap pressure before
validation rejects subsequent reads.
ProfileService.java [260-266]

Referred Code
    ActionListener.wrap(updateResponse -> {
        getVersionedDocument(request.getUid(), ActionListener.wrap(doc -> {
            validateProfileSize(doc, request, maxProfileSize);
            listener.onResponse(AcknowledgedResponse.TRUE);
        }, listener::onFailure));
    }, listener::onFailure)
);
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing Audit Logs: The new enforcement of profile size limits and update operations lack explicit audit
logging of the critical actions and outcomes.

Referred Code
        builder.endObject();
    } catch (IOException e) {
        listener.onFailure(e);
        return;
    }

    doUpdate(
        buildUpdateRequest(request.getUid(), builder, request.getRefreshPolicy(), request.getIfPrimaryTerm(), request.getIfSeqNo()),
        ActionListener.wrap(updateResponse -> {
            getVersionedDocument(request.getUid(), ActionListener.wrap(doc -> {
                validateProfileSize(doc, request, maxProfileSize);
                listener.onResponse(AcknowledgedResponse.TRUE);
            }, listener::onFailure));
        }, listener::onFailure)
    );
}

static void validateProfileSize(VersionedDocument doc, UpdateProfileDataRequest request, ByteSizeValue limit) {
    if (doc == null) {
        return;
    }


 ... (clipped 14 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Edge Case Handling: Size validation happens after update and throws BAD_REQUEST via
ElasticsearchStatusException without visible retry/rollback strategy, which may require
verification for robustness under concurrent updates.

Referred Code
doUpdate(
    buildUpdateRequest(request.getUid(), builder, request.getRefreshPolicy(), request.getIfPrimaryTerm(), request.getIfSeqNo()),
    ActionListener.wrap(updateResponse -> {
        getVersionedDocument(request.getUid(), ActionListener.wrap(doc -> {
            validateProfileSize(doc, request, maxProfileSize);
            listener.onResponse(AcknowledgedResponse.TRUE);
        }, listener::onFailure));
    }, listener::onFailure)
);

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
No Log Context: The new code introduces validation and exceptions but does not add structured logs for
enforcement decisions, making it unclear if sensitive data could be logged elsewhere or if
logs are sufficiently structured.

Referred Code
if (doc == null) {
    return;
}
Map<String, Object> labels = combineMaps(doc.doc.labels(), request.getLabels());
Map<String, Object> data = combineMaps(mapFromBytesReference(doc.doc.applicationData()), request.getData());
ByteSizeValue actualSize = ByteSizeValue.ofBytes(serializationSize(labels) + serializationSize(data));
if (actualSize.compareTo(limit) >= 0) {
    throw new ElasticsearchStatusException(
        Strings.format(
            "cannot update profile [%s] because the combined profile size of [%s] exceeds the maximum of [%s]",
            request.getUid(),
            actualSize,
            limit
        ),
        RestStatus.BAD_REQUEST
    );
}

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
The size validation logic is flawed

The profile size validation occurs after the document has already been updated,
which allows oversized data to be persisted. The logic should be inverted to
validate the potential size before performing the update.

Examples:

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/profile/ProfileService.java [258-266]
        doUpdate(
            buildUpdateRequest(request.getUid(), builder, request.getRefreshPolicy(), request.getIfPrimaryTerm(), request.getIfSeqNo()),
            ActionListener.wrap(updateResponse -> {
                getVersionedDocument(request.getUid(), ActionListener.wrap(doc -> {
                    validateProfileSize(doc, request, maxProfileSize);
                    listener.onResponse(AcknowledgedResponse.TRUE);
                }, listener::onFailure));
            }, listener::onFailure)
        );

Solution Walkthrough:

Before:

// ProfileService.java
public void updateProfileData(UpdateProfileDataRequest request, ActionListener listener) {
    // ... build update request ...

    // 1. Perform the update on the document
    doUpdate(updateRequest, ActionListener.wrap(updateResponse -> {
        // 2. After update succeeds, get the newly updated document
        getVersionedDocument(request.getUid(), ActionListener.wrap(doc -> {
            // 3. Validate the size of the document that is already persisted
            validateProfileSize(doc, request, maxProfileSize);
            listener.onResponse(AcknowledgedResponse.TRUE);
        }, listener::onFailure));
    }, listener::onFailure));
}

After:

// ProfileService.java
public void updateProfileData(UpdateProfileDataRequest request, ActionListener listener) {
    // 1. Get the current document *before* updating
    getVersionedDocument(request.getUid(), ActionListener.wrap(doc -> {
        // 2. Validate what the size WOULD BE if the update were applied
        validateProfileSize(doc, request, maxProfileSize);

        // 3. If validation passes, build and perform the update
        // ... build update request ...
        doUpdate(updateRequest, listener.map(response -> AcknowledgedResponse.TRUE));
    }, e -> {
        // Handle cases where document does not exist (new profile)
        // or other read failures.
        listener.onFailure(e);
    }));
}
Suggestion importance[1-10]: 10

__

Why: This suggestion correctly identifies a critical flaw in the core logic where the profile is updated before its size is validated, which completely defeats the purpose of the feature.

High
Possible issue
Validate profile size before updating

Reverse the logic in updateProfileData to validate the potential profile size
before performing the update, preventing oversized profiles from being saved.

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/profile/ProfileService.java [234-267]

 public void updateProfileData(UpdateProfileDataRequest request, ActionListener<AcknowledgedResponse> listener) {
-    final XContentBuilder builder;
-    try {
-        builder = XContentFactory.jsonBuilder();
-        builder.startObject();
-        {
-            builder.field("user_profile");
+    getVersionedDocument(request.getUid(), ActionListener.wrap(doc -> {
+        validateProfileSize(doc, request, maxProfileSize);
+
+        final XContentBuilder builder;
+        try {
+            builder = XContentFactory.jsonBuilder();
             builder.startObject();
             {
-                if (false == request.getLabels().isEmpty()) {
-                    builder.field("labels", request.getLabels());
+                builder.field("user_profile");
+                builder.startObject();
+                {
+                    if (false == request.getLabels().isEmpty()) {
+                        builder.field("labels", request.getLabels());
+                    }
+                    if (false == request.getData().isEmpty()) {
+                        builder.field("application_data", request.getData());
+                    }
                 }
-                if (false == request.getData().isEmpty()) {
-                    builder.field("application_data", request.getData());
-                }
+                builder.endObject();
             }
             builder.endObject();
+        } catch (IOException e) {
+            listener.onFailure(e);
+            return;
         }
-        builder.endObject();
-    } catch (IOException e) {
-        listener.onFailure(e);
-        return;
-    }
 
-    doUpdate(
-        buildUpdateRequest(request.getUid(), builder, request.getRefreshPolicy(), request.getIfPrimaryTerm(), request.getIfSeqNo()),
-        ActionListener.wrap(updateResponse -> {
-            getVersionedDocument(request.getUid(), ActionListener.wrap(doc -> {
-                validateProfileSize(doc, request, maxProfileSize);
-                listener.onResponse(AcknowledgedResponse.TRUE);
-            }, listener::onFailure));
-        }, listener::onFailure)
-    );
+        final UpdateRequest updateRequest = buildUpdateRequest(
+            request.getUid(),
+            builder,
+            request.getRefreshPolicy(),
+            request.getIfPrimaryTerm(),
+            request.getIfSeqNo()
+        );
+
+        doUpdate(updateRequest, listener.map(updateResponse -> AcknowledgedResponse.TRUE));
+    }, e -> {
+        if (e instanceof ResourceNotFoundException) {
+            // The profile does not exist, so we can proceed with the update as it will create a new one.
+            // The size validation will be implicitly handled as if the original document was empty.
+            updateProfileData(new UpdateProfileDataRequest(
+                request.getUid(),
+                request.getLabels(),
+                request.getData(),
+                -1, // Creates a new document, so no optimistic locking
+                -1, // Creates a new document, so no optimistic locking
+                request.getRefreshPolicy()
+            ), listener);
+        } else {
+            listener.onFailure(e);
+        }
+    }));
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical logic flaw where the profile size is validated after the update, which could persist an oversized profile and render it un-updatable.

High
General
Improve performance with thread-local builders

Improve performance in the serializationSize method by using a ThreadLocal
XContentBuilder to avoid repeated object allocation.

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/profile/ProfileService.java [305-312]

+private static final ThreadLocal<BytesStreamOutput> threadLocalBytesStream = ThreadLocal.withInitial(BytesStreamOutput::new);
+
 static int serializationSize(Map<String, Object> map) {
-    try (XContentBuilder builder = XContentFactory.jsonBuilder()) {
+    final BytesStreamOutput stream = threadLocalBytesStream.get();
+    stream.reset();
+    try (XContentBuilder builder = XContentFactory.jsonBuilder(stream)) {
         builder.value(map);
-        return BytesReference.bytes(builder).length();
+        return builder.bytes().length();
     } catch (IOException e) {
         throw new ElasticsearchException("Error occurred computing serialization size", e); // I/O error should never happen here
     }
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion offers a valid performance optimization by using a ThreadLocal to reuse builders, which reduces object allocation and GC pressure in a frequently called method.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants